-
Notifications
You must be signed in to change notification settings - Fork 39
[김진형]sprint1 #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[김진형]sprint1 #28
The head ref may contain hidden characters: "Basic-\uAE40\uC9C4\uD615-sprint15"
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
진형님 첫번째 PR 제출 고생하셨습니다!
리뷰 드린 코멘트는 모두 필수 반영사항은 아니고 코드에 정답은 없으니
보고 마음에 드시는 방향으로 수정하시면 좋겠습니다!
다음 주차도 화이팅입니다~
- PR에 올려주신 사진과 달리 배포에서는 상하단 배너 판다들이 잘 보이네요👍
- git commit 단위를 나누시는 연습을 해보시면 좋을 것 같아요. index page css 추가, html 수정과 같은 식으로 작게 쪼개시면 시점을 돌리시기도 더 좋답니다.
- 현재 폴더 구조에 15-Sprint-Mission 폴더가 존재하는데 해당 폴더가 없어도 될 것 같아요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 칭찬
파비콘 추가하신 것 좋아요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
해당 레포를 통해 panda-market이라는 것을 알고 있으니 logo.png로 하는게 더 명확한 것 같아요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
귀여운 텍스트네요~ 다만 repo에는 불필요한 파일이 없는 것이 관리하기 좋아요~ 해당 파일은 삭제하시는 것을 추천드려요!
추후 commit 하실때도 필요한 파일만 포함하시는 것을 추천드릴께요~
| </head> | ||
| <body> | ||
| <header> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗️ 수정요청
불필요한 공백은 가독성에 좋지 않습니다~ 붙여쓰시는 것을 추천드려요!
| <header> | ||
|
|
||
| <a href="/"> | ||
| <img src="images/logo/panda-market-logo.png" alt="판다마켓홈" width="153"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
img 태그의 alt 속성은 alternative 라는 의미로, 이미지 파일을 다운로드하는 것에 실패해서 이미지 파일을 보여줄 수 없을 때
어떤 이미지인지 파악할 수 있게 대신 제공되거나, 스크린리더로 읽혀지는 문자를 의미합니다.
따라서 가능한 해당 이미지를 잘 표현할 수 있는 문자열을 넣어주는 것이 좋습니다.
지금의 경우 "판다마켓"이 적절할 것 같아요~
| border-bottom: 1px solid #dfdfdf; | ||
| } | ||
|
|
||
| #loginLinkButton { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
css selector로 id 사용은 지양하시는 것이 좋습니다. id 선택자의 경우 높은 우선순위를 가지고 있어
추후 스타일을 덮어쓰기 어렵게 만듭니다~
그와 반대로 태그는 쉽게 스타일이 덮어 씌워집니다.
따라서 overriding 되기 쉽게 하고 싶으시면 태그 선택자를 그 외에는 클래스명을 이용해주세요.
id는 꼭 필요한 경우 사용하시면 됩니다~
| padding: 11.5px 23px; | ||
| } | ||
|
|
||
| .button:hover { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗️ 수정요청
변수나 클래스명, 이미지명과 같은 이름은 해당 파일이 어떤 역할을 하는 친구인지 나타내는 큰 단서가 되고 코드파악 및 유지보수에 중요한 역할을 합니다. 가능하면 유의미한 이름을 사용해주세요~
버튼은 너무 일반 명사라 어떤 요소인지 알기가 어렵습니다.
| justify-content: center; | ||
| } | ||
|
|
||
| a. { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗️ 수정요청
오타일까요? 오타라면 아래처럼 수정하시는 것을 추천드려요~
| a. { | |
| a { |
| font-size: 16px; | ||
| font-weight: 600; | ||
| border-radius: 8px; | ||
| padding: 11.5px 23px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 여담
css에서의 소수점 사용에 대해 다양한 의견이 있습니다.
예전에는 이러한 소수점이 정수로 변환된다는 것이 정론에 가까웠으나, 다양한 해상도가 나오면서 이러한 소수점도 반영된다는 의견이 있습니다.
정확한 명세로 나온것은 없지만, 저는 소수점 사용을 지양하는 편입니다.
만약 같은 화면 크기임에도 해상도에 따라 소수점이 반영되는 것이 갈린다면, 디자인의 일관성이 떨어지는 것이고 소수점 사용은 가독성에 좋지 못하다고 생각하기 때문입니다~
이와 관련해 다양한 논의들이 있으니 참고해보세요!
https://www.reddit.com/r/css/comments/148y3xm/is_it_ok_to_give_text_the_size_in_decimals_like/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💊 제안
브라우저에서는 진형님이 css를 작성해주신 것처럼 자체적으로 style을 가지고 있습니다.
이는 검사 창에서 사용자 에이전트 스타일시트라는 항목으로 확인하실 수 있습니다.
이러한 스타일은 브라우저 별로 다를 수 있기 때문에 밑바탕을 만들듯 이를 초기화하는 과정이 필요합니다.
목적에 따라 구분해 두는 것이 나중에 찾기 편하기 때문에 css 또한 구분해서 파일을 관리하는 것을 추천드립니다.
예를 들어 위의 브라우저 스타일 시트 초기화 관련 css들은 reset.css 혹은 normalize.css 라는 파일로 관리를 합니다.
구글에 reset.css, normalize.css로 검색해보시면 미리 작성된 css 파일이 있으니 확인해보시고 추가해보세요~
이렇게 하시면 브라우저 기본 스타일 초기화를 위해 반복적으로 css를 작성하실 필요가 없어집니다~
[기본 요구사항]
체크리스트 [기본]
체크리스트 [심화]
스크린샷
멘토에게